fix: fix integration test automated run#472
Conversation
Signed-off-by: Pablo Garay <pagaray@nvidia.com>
Signed-off-by: Pablo Garay <pagaray@nvidia.com>
| github.ref == 'refs/heads/main' | ||
| || github.event_name == 'workflow_dispatch' | ||
| || needs.pre-flight.outputs.is_ci_workload == 'true' | ||
| || needs.pre-flight.outputs.force_run_all == 'true' |
There was a problem hiding this comment.
is_ci_workload already accounts for whether it is on main branch or not
https://github.com/NVIDIA-NeMo/FW-CI-templates/blob/845ca1e302e9592b067c7696e372fa63fd1b8b6f/.github/workflows/_cicd_preflight.yml#L116
Is force_run_all even a part of the expected workflow for these tests? If this only runs in the "main" environment, which I think is just main branch? Then there will never be a force-run-all label.
There was a problem hiding this comment.
I think is just main branch
@chtruong814 I asked Pablo to also allow manual run for any branch. We basically want to run CI without this test for PRs, but have an option to trigger it if needed.
There was a problem hiding this comment.
Btw
|| needs.pre-flight.outputs.force_run_all == 'true'
is consistently used across all the file (e.g. look above lines) & yes it wouldnt have effect though i would suggest we make that change consistently (for consistency) in follow up PR then (removing all other occurrences in other places) - if thats wanted to be removed
There was a problem hiding this comment.
Sorry I guess I'm confused. In the example you shared above, the integration test is running but fails due to the Github environment configuration.
https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19499650277/job/55810558564
So do we want to run this test successfully only on main branch or also allow other branches?
There was a problem hiding this comment.
@pablo-garay where are you expecting this branch variable to be used?
https://github.com/NVIDIA-NeMo/Evaluator/blob/main/.github/workflows/cicd-main.yml#L18
There was a problem hiding this comment.
I stand corrected, my bad. It's been extra hectic busy so didnt get proper time for more thorough testing
"fails due to the Github environment configuration." -> needed a fix, updated now
So do we want to run this test successfully only on main branch or also allow other branches?
please refer to:
where are you expecting this branch variable to be used?
IIUC your question, These are the input params
Signed-off-by: Pablo Garay <pagaray@nvidia.com>
- Add condition to run integration tests when pushing to main branch - Integration tests now run on: scheduled cron, manual triggers, and main branch pushes - PRs continue to skip integration tests as intended
|
|
||
| cicd-integration-tests: | ||
| needs: | ||
| - pre-flight |
There was a problem hiding this comment.
It doesn't need pre-flight anymore right?
There was a problem hiding this comment.
true ; this was remnant of previous approach
Signed-off-by: Pablo Garay <pagaray@nvidia.com>
|
/ok to test dc7dc8e |
Marta already approved ; did one (inconsequential) last change since ; hence merging
|
pablo-garay
deleted the
pablo-garay/fix-integration-test-automated-run2
branch
Integration tests triggering: Plan : to skip them for PRs, but we wanted to run them on main and I'd like to be able to trigger them manually too This would now satisfies requirements: Req 1a: PRs with /ok to test xyz will SKIP integration tests (they use push events to pull-request/* branches, not schedule or workflow_dispatch) Req 1b: Merges to main won't automatically run integration tests (also push events), but scheduled runs will catch them Req 2: Manual workflow_dispatch triggers from any branch will run integration tests Req 3: No more environment protection errors when triggering from feature branches The key insight is that by checking github.event_name directly instead of relying on is_ci_workload, we: - Don't require the workflow to be on main branch for manual triggers - Explicitly exclude PR runs (which are push events) - Allow scheduled runs to work as before <img width="1062" height="400" alt="image" src="https://github.com/user-attachments/assets/bc85561c-5b47-4cf3-bed0-8e4de8c8687f" /> <img width="324" height="267" alt="image" src="https://github.com/user-attachments/assets/4830137b-ef9a-45f5-b05f-c082c0dd0d56" /> Fixed integration tests triggering logic: - Skip on PRs (including /ok to test) - Run on main branch pushes (when PRs merge) - Run on scheduled cron jobs (daily) - Run on manual workflow_dispatch triggers (from any branch) Tested: For: - Use workflow from: pablo-garay/fix-integration-test-automated-run2 - Branch to run tests on: pablo-garay/fix-integration-test-automated-run2 https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19550694641 another test: https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19552356088/job/55986938425 --------- Signed-off-by: Pablo Garay <pagaray@nvidia.com> Signed-off-by: Lawrence Lane <llane@nvidia.com>
Integration tests triggering: Plan : to skip them for PRs, but we wanted to run them on main and I'd like to be able to trigger them manually too This would now satisfies requirements: Req 1a: PRs with /ok to test xyz will SKIP integration tests (they use push events to pull-request/* branches, not schedule or workflow_dispatch) Req 1b: Merges to main won't automatically run integration tests (also push events), but scheduled runs will catch them Req 2: Manual workflow_dispatch triggers from any branch will run integration tests Req 3: No more environment protection errors when triggering from feature branches The key insight is that by checking github.event_name directly instead of relying on is_ci_workload, we: - Don't require the workflow to be on main branch for manual triggers - Explicitly exclude PR runs (which are push events) - Allow scheduled runs to work as before <img width="1062" height="400" alt="image" src="https://github.com/user-attachments/assets/bc85561c-5b47-4cf3-bed0-8e4de8c8687f" /> <img width="324" height="267" alt="image" src="https://github.com/user-attachments/assets/4830137b-ef9a-45f5-b05f-c082c0dd0d56" /> Fixed integration tests triggering logic: - Skip on PRs (including /ok to test) - Run on main branch pushes (when PRs merge) - Run on scheduled cron jobs (daily) - Run on manual workflow_dispatch triggers (from any branch) Tested: For: - Use workflow from: pablo-garay/fix-integration-test-automated-run2 - Branch to run tests on: pablo-garay/fix-integration-test-automated-run2 https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19550694641 another test: https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19552356088/job/55986938425 --------- Signed-off-by: Pablo Garay <pagaray@nvidia.com> Signed-off-by: Lawrence Lane <llane@nvidia.com>
3 participants
Integration tests triggering:
Plan : to skip them for PRs, but we wanted to run them on main and I'd like to be able to trigger them manually too
This would now satisfies requirements:
Req 1a: PRs with /ok to test xyz will SKIP integration tests (they use push events to pull-request/* branches, not schedule or workflow_dispatch)
Req 1b: Merges to main won't automatically run integration tests (also push events), but scheduled runs will catch them
Req 2: Manual workflow_dispatch triggers from any branch will run integration tests
Req 3: No more environment protection errors when triggering from feature branches
The key insight is that by checking github.event_name directly instead of relying on is_ci_workload, we:
Fixed integration tests triggering logic:
Tested:
For:
https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19550694641
another test: https://github.com/NVIDIA-NeMo/Evaluator/actions/runs/19552356088/job/55986938425